bugfix(globaldata): Fix the handling of documents folder redirection by using SHGetKnownFolderPath()#2479
Conversation
Mauller
added
Bug
|
| Filename | Overview |
|---|---|
| Generals/Code/GameEngine/Source/Common/GlobalData.cpp | Replaces the inline SHGetSpecialFolderPath call with a new static helper BuildUserDataPathFromIni() that dynamically resolves SHGetKnownFolderPath via GetProcAddress and falls back to the legacy API; CreateDirectory is now called unconditionally even when the path is empty. |
| GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp | Mirrors the Generals change with a non-static BuildUserDataPathFromRegistry() helper; same CreateDirectory-on-empty-path concern; function is declared non-static despite not accessing any instance members. |
| Generals/Code/GameEngine/Include/Common/GlobalData.h | Adds static AsciiString BuildUserDataPathFromIni() declaration to the private section — clean and consistent. |
| GeneralsMD/Code/GameEngine/Include/Common/GlobalData.h | Adds AsciiString BuildUserDataPathFromRegistry() (non-static) — should be static to match its Generals counterpart and reflect that the function uses no instance state. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["BuildUserDataPath helper called"] --> B["GetModuleHandleA shell32.dll"]
B --> C{shell32 loaded?}
C -- No --> F["SHGetSpecialFolderPath fallback"]
C -- Yes --> D["GetProcAddress SHGetKnownFolderPath"]
D --> E{Function found Vista+?}
E -- No --> F
E -- Yes --> G["SHGetKnownFolderPath FOLDERID_Documents"]
G --> H{SUCCEEDED and pszPath?}
H -- Yes --> I["translate + CoTaskMemFree"]
H -- No --> J["myDocumentsDirectory empty"]
F --> K{SHGetSpecialFolderPath ok?}
K -- Yes --> L["myDocumentsDirectory = temp"]
K -- No --> J
I --> M{myDocumentsDirectory empty?}
L --> M
J --> M
M -- No --> N["Append backslash + leafName + backslash"]
M -- Yes --> O["return empty string"]
N --> P["return full path"]
P --> Q["CreateDirectory at call site"]
O --> Q
Prompt To Fix All With AI
This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Include/Common/GlobalData.h
Line: 579
Comment:
**`BuildUserDataPathFromRegistry` should be `static` for consistency**
`BuildUserDataPathFromRegistry()` does not access any instance members — it uses only `GetStringFromRegistry` (a free function) and local variables. Its counterpart in the Generals build, `BuildUserDataPathFromIni`, is correctly declared `static`. Marking this one `static` as well makes the intent explicit, avoids the implicit `this` capture, and keeps the two codebases symmetric.
```suggestion
static AsciiString BuildUserDataPathFromRegistry();
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/Common/GlobalData.cpp
Line: 1175-1176
Comment:
**`CreateDirectory` called unconditionally on potentially empty path**
`BuildUserDataPathFromIni()` returns an empty `AsciiString` when both shell APIs fail. The call site then passes `""` to `CreateDirectory`, which will silently return `ERROR_PATH_NOT_FOUND` — harmless in practice since the return value is not checked, but it is a behavioural change from the original code where `CreateDirectory` was only called inside the successful `SHGetSpecialFolderPath` branch. Consider guarding the call:
```cpp
TheWritableGlobalData->m_userDataDir = BuildUserDataPathFromIni();
if (!TheWritableGlobalData->m_userDataDir.isEmpty())
CreateDirectory(TheWritableGlobalData->m_userDataDir.str(), nullptr);
```
The same pattern applies to `GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp` line 1042.
How can I resolve this? If you propose a fix, please make it concise.Reviews (8): Last reviewed commit: "bugfix(globaldata): Fix the handling of ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Ah i didn't check building with VC6, will work on this further |
Mauller
force-pushed
the
Mauller/fix-documents-folder-redirection
branch
from
0c251b8 to
9aeb428
Compare
|
Updated by making the fix conditoinal so it only works in non VC6 builds |
Mauller
changed the title
Mauller
force-pushed
the
Mauller/fix-documents-folder-redirection
branch
from
9aeb428 to
0378670
Compare
|
Cleaned up the implementations by replicating the whole code block |
|
Removed controversial as this fix does not work in VC6 in the end anyway, so i had to make the fix conditional at compilation |
| AsciiString myDocumentsDirectory; | ||
| myDocumentsDirectory.translate(UnicodeString(pszPath)); | ||
|
|
||
| if (myDocumentsDirectory.getCharAt(myDocumentsDirectory.getLength() -1) != '\\') |
There was a problem hiding this comment.
Can hardcoded path separators be an issue for (future) linux support?
There was a problem hiding this comment.
All of this will need altering to support linux in the future, the main functions for getting the documents folder are windows API.
Mauller
force-pushed
the
Mauller/fix-documents-folder-redirection
branch
from
0378670 to
f08392e
Compare
|
This was pain, but it works for building in VC6. |
Mauller
force-pushed
the
Mauller/fix-documents-folder-redirection
branch
from
f08392e to
122c1ad
Compare
|
Fixed accidental variable shadowing in the legacy pathway. |
Mauller
force-pushed
the
Mauller/fix-documents-folder-redirection
branch
2 times, most recently
from
50162a8 to
f661212
Compare
|
Do Generals and Zero Hour provide the correct data for both methods even though their code only uses one path? If so, can we not unify this code so it tries one path first and falls back to the second path? |
No, zero hour only has the registry entry and generals only has the ini entry. |
Mauller
force-pushed
the
Mauller/fix-documents-folder-redirection
branch
from
f661212 to
edb5c60
Compare
|
Updated based on feedback. |
Okay, so then if you unified the code to try the registry first then fall back to ini or vice versa, the code would cover both games and can be made common? |
Yes but that is beyond the scope of this change. Mauller could do a Unify change before this change however to streamline the behavior. |
| HRESULT hr = pSHGetKnownFolderPath(FOLDERID_Documents, KF_FLAG_DEFAULT, nullptr, &pszPath); | ||
|
|
||
| if (SUCCEEDED(hr) && pszPath) { | ||
| myDocumentsDirectory.translate(UnicodeString(pszPath)); |
There was a problem hiding this comment.
Explicit UnicodeString construction should not be necessary.
| } | ||
| } | ||
|
|
||
| if (myDocumentsDirectory.isEmpty()) |
There was a problem hiding this comment.
Can be simplified with
if (!myDocumentsDirectory.isEmpty())
{
...
}
return myDocumentsDirectory;
…by using SHGetKnownFolderPath() - Vista+ required
Mauller
force-pushed
the
Mauller/fix-documents-folder-redirection
branch
from
edb5c60 to
94ece27
Compare
|
Tweaked based on feedback, should be good to go now. |
4 participants
This PR improves folder redirection handling that can occur due to Group Policy or OneDrive redirecting the users documents folder.
This only works for non retail builds as it required a function not supported under VC6 as it only appeared in Vista onwards.
SHGetKnownFolderPath() has better handling of folder redirection built into it compared to the legacy SHGetSpecialFolderPath()
Generals and zero hour use different ways to obtain the game folder name which is why the code is different between them.
Edit - To support building in VC6 without compilation conditional, SHGetSpecialFolderPath is obtained as a function pointer if it is available.
Defines for the GUID of the documents folder path are also added locally as these do not exist pre Vista.